Implement match and match cycle repos - #74
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
5470d2c to
dbbd19b
Compare
Graphite Automations"Request reviewers once CI passes" took an action on this PR • (08/10/26)2 reviewers were added to this PR based on Henry Chen's automation. |
|
Code looks good, although I wonder if the match cycle repo could use some per-field update methods like match has. Also, we planning on writing any tests for these? Not sure if there's another ticket for them |
dbbd19b to
b38d8dc
Compare
b38d8dc to
1eefc69
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
thanks for the feedback il include tests in the next PR. Will update this one with the per field methods |
7421818 to
6e2e2a1
Compare
adb4bd4 to
3df3274
Compare
3df3274 to
b63a2c8
Compare
|
One other thing is that you're building out everything together layer by layer, which makes sense with how we planned it out and with AI implementing it, but it would be easier to build it out feature by feature once we get it to a little more stable state. Like having the basics set, and then adding filtering and feedback later down, instead of including it from the start. |
b63a2c8 to
dedfb71
Compare
dedfb71 to
33ce420
Compare
66cc820 to
5294cd8
Compare
33ce420 to
3c16f9e
Compare
3c16f9e to
081d755
Compare
5294cd8 to
8156404
Compare
|
❌ The last analysis has failed. |
|
| } | ||
|
|
||
| @Override | ||
| public Optional<MatchCycle> setMatchCycleDraft(Integer id, boolean isDraft) { |
There was a problem hiding this comment.
Nit: better titled as setMatchCycleIsDraft, this way it's clearer that it's a Boolean switch.
| public MatchCycle createMatchCycle(MatchCycle matchCycle) { | ||
| String sql = """ | ||
| INSERT INTO "match_cycles" ( | ||
| "period", |
There was a problem hiding this comment.
Postgres has range types we can use to do range searches, that would probably be a better type fit for this field. This would probably be more effective than shoehorning the range into a single format. Unsure how much we'll need that in the future though.
| .optional(); | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
Based on the discussions we had about the API, since a lot of these matches might be marked as bad by Henry in the beginning especially, may be good to have an update score function here. (I.e. so Henry can mark it as -100 or whatever)
| String sql = """ | ||
| INSERT INTO "matches" ( | ||
| "id", | ||
| "member_a_id", |
There was a problem hiding this comment.
A few thoughts here:
- would we benefit from including member emails so the queries would be simpler? I'm assuming member ids are not indexed right now so it probably wouldn't make a difference performance wise. We could also include both in the table
- We should have some kind of unique constraint or at least index on the pairing of member an and b's ids/emails. So we can easily look up "have a and b matched before? Or have b and a matched before"? Because that would be an instantly disqualifying factor.
3 participants
No description provided.